Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-40002: Try to support pydantic v1 and v2 #866

Merged
merged 18 commits into from
Jul 19, 2023
Merged

DM-40002: Try to support pydantic v1 and v2 #866

merged 18 commits into from
Jul 19, 2023

Conversation

timj
Copy link
Member

@timj timj commented Jul 14, 2023

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

timj added 5 commits July 14, 2023 16:41
We always tend to install the optionals for testing but it
turns out the import tests were not working properly.

Consider removing S3 completely.
Somehow pydantic v1 did not notice.
Still some failures. Lots of deprecation warnings.
AstropyArrow and Dataframe use classes that are not part of
the core butler dependencies. This leads to the tests failing
in a minimal environment. Replace with two storage classes
that are available to the core environment.
This enables pydantic v2
@timj timj force-pushed the tickets/DM-40002 branch 2 times, most recently from d50053e to a4fcc53 Compare July 18, 2023 00:08
This is meant to be the rquivalent of using __setattr__
and __field_set__ directly. The latter has been renamed in
pydantic v2
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Patch coverage: 58.10% and project coverage change: -0.22% ⚠️

Comparison is base (f8470df) 87.94% compared to head (1ce5be8) 87.72%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #866      +/-   ##
==========================================
- Coverage   87.94%   87.72%   -0.22%     
==========================================
  Files         273      274       +1     
  Lines       35896    35937      +41     
  Branches     7510     7537      +27     
==========================================
- Hits        31567    31526      -41     
- Misses       3170     3241      +71     
- Partials     1159     1170      +11     
Files Changed Coverage Δ
python/lsst/daf/butler/registry/tests/_registry.py 98.10% <ø> (ø)
python/lsst/daf/butler/registry/wildcards.py 52.48% <22.72%> (-2.70%) ⬇️
tests/test_butler.py 97.78% <33.33%> (ø)
python/lsst/daf/butler/_compat.py 33.92% <33.92%> (ø)
tests/test_parquet.py 97.64% <38.46%> (-1.52%) ⬇️
tests/test_postgresql.py 94.78% <60.00%> (-1.62%) ⬇️
python/lsst/daf/butler/core/logging.py 90.51% <65.71%> (-4.94%) ⬇️
python/lsst/daf/butler/core/dimensions/_records.py 83.85% <66.66%> (-2.21%) ⬇️
python/lsst/daf/butler/core/datastoreRecordData.py 86.90% <71.42%> (-2.87%) ⬇️
...hon/lsst/daf/butler/core/dimensions/_coordinate.py 87.15% <71.42%> (-0.42%) ⬇️
... and 12 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

timj added 2 commits July 18, 2023 13:59
There is clearly some issue with the RootModel constructor
that we will need to sort out.
This required that we allow unused igores since the location
of the ignores depends on the version of pydantic and mypy
always picks up the first definition it sees even if that
is a pydantic V2 code path and pydantic is v1.
Copy link
Contributor

@andy-slac andy-slac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK after a quick look, one question is whether _BaseModelCompat.model_construct could be changed to avoid if/else in many model classes.

from moto import mock_s3 # type: ignore[import]
except ImportError:
boto3 = None

def mock_s3(cls): # type: ignore[no-untyped-def]
def mock_s3(*args: Any, **kwargs: Any) -> Any: # type: ignore[no-untyped-def]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect you can drop type: ignore here?

python/lsst/daf/butler/_compat.py Show resolved Hide resolved
python/lsst/daf/butler/_compat.py Show resolved Hide resolved
python/lsst/daf/butler/_quantum_backed.py Outdated Show resolved Hide resolved
@timj
Copy link
Member Author

timj commented Jul 18, 2023

@andy-slac you are right that I could implement v1 model_construct in _BaseModelCompat and just use it in the same way for both pydantic. Then I could even make v2 do the explicit setter approach if we decided we didn't want to use the base v2 base class model_construct. I will change.

timj added 3 commits July 19, 2023 15:39
If the BaseModel.construct() method is slow we can still
reimplement it as was done in the individual direct() methods
but with the direct() methods still being much simpler.
The previous version used by the direct() methods
would be something like:

        def model_construct(cls, _fields_set: set[str] | None = None, **values: Any) -> Self:
            node = cls.__new__(cls)
            setter = object.__setattr__
            for k, v in values.items():
                setter(node, k, v)
            if _fields_set is None:
                _fields_set = set(values.keys())
            setter(node, "__fields_set__", _fields_set)
            return node

Testing with 100MB graph indicates that construct() might be
fractionally slower than implementing it ourselves as above
but it is not conclusive. The direct() method only accounts
for 14% of the graph load time.
@timj timj merged commit 1fe3838 into main Jul 19, 2023
14 of 16 checks passed
@timj timj deleted the tickets/DM-40002 branch July 19, 2023 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants